-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose and document a simpler public API for simplify expressions #3719
Conversation
/// when performing type coercion. | ||
pub fn simplify_expr( | ||
expr: Expr, | ||
schema: DFSchemaRef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since SimplifyContext::new
needs a Vec<&'Arc<DFSchema>
, so I change the input schema
to Arc<DFSchema>
to avoid clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense -- the SimplifyContext constructor is somewhat akward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working on #3741 I found having a signature of schema: &DFSchemaRef
was actually more ergnonmic. What do you think?
We have to rerun the CI anyways give the github nonsense earlier today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
@alamb, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ygf11 -- this looks great. I think it would be ready to merge after addressing @andygrove 's comments about testing style.
🏆
/// when performing type coercion. | ||
pub fn simplify_expr( | ||
expr: Expr, | ||
schema: DFSchemaRef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense -- the SimplifyContext constructor is somewhat akward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great -- thank you @ygf11
Github actions is having issues I think -- the CI failures are not related to changes in this PR |
Yes, I see other pr also meets the same ci error 😅. |
Benchmark runs are scheduled for baseline = 1e1de82 and contender = fef45e7. fef45e7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3709.
Rationale for this change
This api is very useful for other projects which depends on datafusion, like filter pushdown, default-value when insert etc.
What changes are included in this PR?
Expose and document a simpler public API for simplify expressions
Are there any user-facing changes?
Added a public method
simplify_expr
.